-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor the source spec validation #1122
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
b6924e5
to
1a77d5a
Compare
73e8d8c
to
bc97cf7
Compare
@@ -591,7 +591,7 @@ func TestManagingReconciler(t *testing.T) { | |||
|
|||
// test case 7: the reconciler-manager should mount the git-creds volumes again if the auth type requires a git secret | |||
nt.T.Log("Switch the auth type gcpserviceaccount to ssh") | |||
nt.MustMergePatch(rs, `{"spec":{"git":{"auth":"ssh","secretRef":{"name":"git-creds"}}}}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with this stricter validation, but just wanted to point out that this could be temporarily disruptive for some users if they have gcpServiceAccountEmail set for other auth types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was also thinking about it. It sounds like we don't have a unified rule to validate the spec.
Currently, the secret ref is allowed only when it is required by certain auth types, but gcpServiceAccountEmail doesn't have such requirement.
On the other hand, spec.git/spec.oci/spec.helm can co-exist even it is not the corresponding source type.
For secret ref, it makes sense to be restrictive because the presence will trigger a copy of the Secret in the c-m-s Namespace. Due to the restriction, this PR makes a consistent behavior between secret ref and gcpServiceAccountEmail. From that perspective, I feel it is okay to ask users to update their configs to follow the same rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making validation stricter is considered breaking. Agreed that this is inconsistent, but we should try our best to avoid breaking users. Unless allowing gcpServiceAccountEmail has huge downside, we should just keep allowing it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about also loosening the restriction on the secretRef validation? Basically, any field that is not needed will be ignored.
That won't break any users, but it makes all fields be validated in a consistent way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loosening it has the downside of causing the Secret to be copied over to c-m-s as you mentioned, but I think that might be fine if we prefer keeping the rule consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can copy it over only when it is used by certain auth types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Assuming this is going to be cherry picked, I don't think we need to alter the behavior with this change. I think it would be preferable to minimize the changes for the cherry pick
If desired we can change the behavior for the next minor release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was going to say it doesn't need to be cherrypicked if we don't add more validations.
This can wait until the next minor release.
3d11c4f
to
c0a746a
Compare
When fields in RSync spec is not used, some are allowed to be present, such as `spec.git`, `spec.oci`, `spec.helm`, and `spec.xxx.gcpServiceAccountEmail`. However, `spec.xxx.secretRef` is not allowed when it is not used. The reconciler-manager already checks if it is the proper auth type before copying to the config-management-system Namespace, so it is okay to loosen the validation for this field. This commit also refactors the auth types to reduce code duplication.
This commit covers more failure scenarios with the redudant fields validaiton.